-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Terraform, GKE - Option to create regional cluster as well as option to create autoscaling nodepool #2813
Conversation
Build Succeeded 👏 Build Id: 3751761c-f0ee-4470-9640-9fe9adb505b6 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
default = "" | ||
description = "The GCP location to create the cluster in" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just deprecate zone
? IMO yes, but would like input from @roberthbailey. It would look like:
- on new line 67, change the default to
"us-west1-c"
- on new line 72, change the default to
""
- on new line 73, let's say
"The GCP zone to create the cluster in (deprecated, use 'location')"
It looks like there is no formal deprecation procedure (hashicorp/terraform#18381) though, so I think to actually enforce the deprecation we'd have to add output to the module if zone
were set.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the plan is to
make sure we keep things backwards compatible for at least 1 release and give folks a warning in the release notes that the variable name is changing.
So adding a deprecation note does follow that. But does this mean that zone
would take precedence over location
for the 1 release before being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the change I just made for this comment, GCP_CLUSTER_ZONE
and GCP_CLUSTER_LOCATION
will always be set. What would be the correct way to go about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCP_CLUSTER_ZONE
and GCP_CLUSTER_LOCATION
are both set, but I think the change you just made to gcloud-terraform-install
sets only location, right?
The logic is a little fussy, but I think we should check:
- if
location
is set but notzone
, uselocation
(good path) - if
zone
is set but notlocation
, usezone
and warn (deprecated path) - if
location
andzone
are set but equal, use either and warn. - if both are set, and not equal, throw an error
I'm not exactly sure how possible that is in .tf, but it seems like the right approach. If it's too much effort, though, holler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add back zone
to gcloud-terraform-install
so that both variables will be available. As for adding warnings and errors, not sure if that is possible within the terraform files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to add zone
back in, I was pointing out that the Makefile is only sending the one - so there's no conflict between them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so I made it so that within the tf file, zone will be empty unless explicitly set. And it will only use zone if the variable is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see this comment thread earlier but I think where you ended up looks good - we use location and we can add to the release notes that folks should switch their configs to use location but we continue to support zone for N releases (maybe 1, maybe a few) to give people time to migrate their configs.
We don't have a formal policy about compatibility of terraform configs (or helm for that matter) but we do try to do our best not to have knife edge changes that would break users upgrading across a single release.
default = "" | ||
description = "The GCP location to create the cluster in" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCP_CLUSTER_ZONE
and GCP_CLUSTER_LOCATION
are both set, but I think the change you just made to gcloud-terraform-install
sets only location, right?
The logic is a little fussy, but I think we should check:
- if
location
is set but notzone
, uselocation
(good path) - if
zone
is set but notlocation
, usezone
and warn (deprecated path) - if
location
andzone
are set but equal, use either and warn. - if both are set, and not equal, throw an error
I'm not exactly sure how possible that is in .tf, but it seems like the right approach. If it's too much effort, though, holler.
I think it's better to also wire these options up to the example module and update the website page in case customer is following the instruction to create the cluster |
Build Succeeded 👏 Build Id: 8292f534-0b3d-4939-a569-7254fab8137b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Failed 😱 Build Id: 87ddce9a-8727-4a94-b492-2d6585e6919e To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 8d675391-e10e-49f9-babb-4fb855048b4e The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 53960b90-45ee-4cf1-908a-769d0ef30e3d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@@ -56,7 +60,7 @@ resource "null_resource" "test-setting-variables" { | |||
|
|||
resource "google_container_cluster" "primary" { | |||
name = local.name | |||
location = local.zone | |||
location = local.zone == "" ? local.location : local.zone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this prioritize location.
It looks like warnings are not straightforward, so I think skipping that is fine. But at the very least, let's add an error if they're both non-empty and mismatched. I think it can be done like: https://medium.com/codex/terraform-variable-validation-b9b3e7eddd79
Later we can start erroring when zone
is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make the location a priority, do we still want location to have a default value? This would also effect the validation done on the variable.
As for cross variable validation, it's currently not supported but there seems to be hacky ways around it if we really need the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So zone no longer has a default and will only be used as the location if zone is not empty.
@@ -53,9 +53,14 @@ variable "enable_image_streaming" { | |||
default = "true" | |||
} | |||
|
|||
variable "zone" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to update the website page to indicate the change. Noted for changes to the website, we generally gate them so that they do not appear until the next release. Even though this doesn't require new features in Agones, it still makes sense to wait to publish this guidance until our next release.
https://agones.dev/site/docs/contribute/#within-a-page describes how to add the feature gate tags to the website pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I think, not sure if I've done it correctly or not.
Build Succeeded 👏 Build Id: cc92fd64-99a6-420d-ba15-244bfb7a1b10 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@@ -96,6 +96,14 @@ Configurable parameters: | |||
- gameserver_maxPort - the upper bound of the port range which gameservers will listen on (default is "8000") | |||
- gameserver_namespaces - a list of namespaces which will be used to run gameservers (default is `["default"]`). For example `["default", "xbox-gameservers", "mobile-gameservers"]` | |||
- force_update - whether or not to force the replacement/update of resource (default is true, false may be required to prevent immutability errors when updating the configuration) | |||
{{% feature publishVersion="1.28" %}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should add another version-guarded section at line 90 to mention that zone
is deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added feature expiry
to the zone. Not sure if this is the best way to do it.
Build Succeeded 👏 Build Id: c57fee7e-6da0-40e6-a275-34cf0327d970 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed while reviewing your change that in build/includes/terraform.mk
there is a lot of duplication between the gcloud-terraform-cluster
and gcloud-terraform-install
targets. If you agree and think there would be a clean way to simplify them, that would be a good change for a follow-up PR.
default = "" | ||
description = "The GCP location to create the cluster in" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see this comment thread earlier but I think where you ended up looks good - we use location and we can add to the release notes that folks should switch their configs to use location but we continue to support zone for N releases (maybe 1, maybe a few) to give people time to migrate their configs.
We don't have a formal policy about compatibility of terraform configs (or helm for that matter) but we do try to do our best not to have knife edge changes that would break users upgrading across a single release.
@@ -30,7 +30,8 @@ variable "cluster" { | |||
type = map | |||
|
|||
default = { | |||
"zone" = "us-west1-c" | |||
"location" = "us-west1-c" | |||
"zone" = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove zone from here to only have location in our sample configs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can. There isn't really a reason to have zone here anymore.
Build Succeeded 👏 Build Id: 7742b8c1-88bd-416e-b68d-bd033161b5be The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: ebe142e8-7f35-4faa-9d51-d17e206e7cc9 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 004ee176-1a2a-4d56-9de1-244138d6856d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: aa55b7a1-9a91-4058-8d94-cb8b9b5c8dcc The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
* Makefile changes for adding location variable * added autoscale parameters to Makefile and README * Markdown fix in readme * Changed LOCATION to always be set with ZONE as default * use only if the variable has a value * fixed extraneous characters * update gke terraform exmaple module * Added autoscale to example cluster and added to website docs * Added defaults and feature expiry * Remove zone from gke/variable.tf file.
Build Succeeded 👏 Build Id: 6ff6b1e1-43cd-46b8-90d4-83c929dee02b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chiayi, gongmax, roberthbailey, zmerlynn The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / Why we need it:
It adds configuration to terraform so that regional clusters can be created.
It also added autoscaling option for
default
nodepools.Which issue(s) this PR fixes:
Closes #1441
Closes #1467
Special notes for your reviewer:
I've also made changes to the README for /build to include the parameters.